Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deploy RDMA CNI as a part of SR-IOV Operator #661

Merged
merged 1 commit into from
Jul 11, 2024

Conversation

e0ne
Copy link
Collaborator

@e0ne e0ne commented Mar 19, 2024

No description provided.

Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@coveralls
Copy link

coveralls commented Mar 19, 2024

Pull Request Test Coverage Report for Build 9858421191

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 5 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.03%) to 40.952%

Files with Coverage Reduction New Missed Lines %
controllers/generic_network_controller.go 5 74.53%
Totals Coverage Status
Change from base Build 9774956990: -0.03%
Covered Lines: 5585
Relevant Lines: 13638

💛 - Coveralls

@e0ne e0ne marked this pull request as ready for review March 19, 2024 11:50
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

hack/env.sh Outdated Show resolved Hide resolved
hack/env.sh Outdated Show resolved Hide resolved
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

Copy link
Collaborator

@ykulazhenkov ykulazhenkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

Copy link
Member

@zeeke zeeke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

github-actions bot commented Apr 3, 2024

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@e0ne e0ne closed this Apr 3, 2024
@e0ne e0ne reopened this Apr 3, 2024
Copy link

github-actions bot commented Apr 3, 2024

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

Copy link

github-actions bot commented Apr 3, 2024

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@e0ne
Copy link
Collaborator Author

e0ne commented Apr 15, 2024

@SchSeba could you please review this PR?

Copy link
Collaborator

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the super late review.

I add some small comments

@@ -92,6 +92,23 @@ spec:
mountPath: /host/etc/os-release
readOnly: true
{{- end }}
{{- if ne .RDMACNIImage "" }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: please can the if statement be the same as {{- if .OVSCNIImage }}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

- name: rdma-cni
image: {{.RDMACNIImage}}
command:
- /bin/sh
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please let's not have a copy like this here.

can we create an entrypoint.sh and use it in the image for the rdma-cni repo (I can review and approve it)

the same way we do for the sriov-cni

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hack/env.sh Outdated
@@ -13,6 +15,8 @@ else
[ -z $SRIOV_INFINIBAND_CNI_IMAGE ] && echo "SRIOV_INFINIBAND_CNI_IMAGE is empty but SKIP_VAR_SET is set" && exit 1
# check that OVS_CNI_IMAGE is set to any value, empty string is a valid value
[ -z ${OVS_CNI_IMAGE+set} ] && echo "OVS_CNI_IMAGE is empty but SKIP_VAR_SET is set" && exit 1
# check that RDMA_CNI_IMAGE is set to any value, empty string is a valid value
[ -z ${$RDMA_CNI_IMAGE+set} ] && echo "RDMA_CNI_IMAGE is empty but SKIP_VAR_SET is set" && exit 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please do something like we did for the ovs-cni

# ensure that OVS_CNI_IMAGE is set, empty string is a valid value
OVS_CNI_IMAGE=${OVS_CNI_IMAGE:-}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

Copy link
Collaborator

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just two small comments and we should be good to go nice work

hack/env.sh Outdated
@@ -11,6 +13,8 @@ if [ -z $SKIP_VAR_SET ]; then
else
# ensure that OVS_CNI_IMAGE is set, empty string is a valid value
OVS_CNI_IMAGE=${OVS_CNI_IMAGE:-}
# ensure that RDMA_CNI_IMAGE is set, empty string is a valid value
$RDMA_CNI_IMAGE=${$RDMA_CNI_IMAGE:-}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$RDMA_CNI_IMAGE= -> RDMA_CNI_IMAGE=

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

bindata/manifests/daemon/daemonset.yaml Show resolved Hide resolved
Copy link
Collaborator

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

/hold

please lets wait for #700 and release cut before we merge this

@adrianchiris
Copy link
Collaborator

@e0ne please rebase so we can give it a final review and merge.

Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@e0ne
Copy link
Collaborator Author

e0ne commented Jun 25, 2024

@e0ne please rebase so we can give it a final review and merge.

done

hack/env.sh Outdated
@@ -3,6 +3,8 @@ if [ -z $SKIP_VAR_SET ]; then
export SRIOV_INFINIBAND_CNI_IMAGE=${SRIOV_INFINIBAND_CNI_IMAGE:-ghcr.io/k8snetworkplumbingwg/ib-sriov-cni}
# OVS_CNI_IMAGE can be explicitly set to empty value, use default only if the var is not set
export OVS_CNI_IMAGE=${OVS_CNI_IMAGE-ghcr.io/k8snetworkplumbingwg/ovs-cni-plugin}
# RDMA_CNI_IMAGE can be explicitly set to empty value, use default only if the var is not set
export RDMA_CNI_IMAGE=${RDMA_CNI_IMAGE-ghcr.io/k8snetworkplumbingwg/rdma-cni}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please align indentation.

hack/env.sh Outdated
@@ -13,6 +15,8 @@ if [ -z $SKIP_VAR_SET ]; then
else
# ensure that OVS_CNI_IMAGE is set, empty string is a valid value
OVS_CNI_IMAGE=${OVS_CNI_IMAGE:-}
# ensure that RDMA_CNI_IMAGE is set, empty string is a valid value
RDMA_CNI_IMAGE=${$RDMA_CNI_IMAGE:-}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please align indentation.

Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

Copy link
Collaborator

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, please align indent as @rollandf commented and we can merge.

Copy link

github-actions bot commented Jul 2, 2024

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

Copy link
Collaborator

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@e0ne can you rebase against master ?

@@ -103,6 +103,7 @@ images:
sriovCni: ghcr.io/k8snetworkplumbingwg/sriov-cni
ibSriovCni: ghcr.io/k8snetworkplumbingwg/ib-sriov-cni
ovsCni: ghcr.io/k8snetworkplumbingwg/ovs-cni-plugin
rdmaCni: ghcr.io/k8snetworkplumbingwg/rdma-cni
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please update helm chart update script :

OVS_CNI_TAG=$(get_latest_github_tag k8snetworkplumbingwg ovs-cni)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

github-actions bot commented Jul 8, 2024

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

Copy link
Collaborator

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

nice work!

@SchSeba
Copy link
Collaborator

SchSeba commented Jul 8, 2024

/hold

Hi @e0ne please check the k8s CI looks like the helm deploy doesn't work

@github-actions github-actions bot added the hold label Jul 8, 2024
@SchSeba
Copy link
Collaborator

SchSeba commented Jul 8, 2024

Signed-off-by: Ivan Kolodiazhnyi <ikolodiazhny@nvidia.com>
Copy link

github-actions bot commented Jul 9, 2024

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@e0ne
Copy link
Collaborator Author

e0ne commented Jul 9, 2024

/hold

Hi @e0ne please check the k8s CI looks like the helm deploy doesn't work

fixed a typo

@e0ne
Copy link
Collaborator Author

e0ne commented Jul 9, 2024

please add the image here

https://github.com/k8snetworkplumbingwg/sriov-network-operator/blob/master/hack/run-e2e-conformance-virtual-cluster.sh#L387

why do we need to to it?

@SchSeba
Copy link
Collaborator

SchSeba commented Jul 9, 2024

/hold cancel

@github-actions github-actions bot removed the hold label Jul 9, 2024
@SchSeba
Copy link
Collaborator

SchSeba commented Jul 11, 2024

Hi @adrianchiris can you please approve so we can merge this one?

@adrianchiris adrianchiris merged commit c3ba0f3 into k8snetworkplumbingwg:master Jul 11, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants